Skip to content

Conversation

lorenzofelletti
Copy link

Motivation

When using SimpleProcessor one may want to have strict unmarshalling for the function config and throw an error if any unknown field is supplied in the configuration.

The current behaviour does not allow this flexibility. Adding strict option to LoadFunctionConfig and adding a Strict field to SimpleProcessor allows to programmatically change the unmarshalling behaviour, while keeping the current behaviour as the default.

Changes

  • Introduced a new boolean field Strict in the SimpleProcessor struct to control strict unmarshalling behavior when loading function configurations.
  • Updated the LoadFunctionConfig function to accept the strict parameter, allowing it to use strict unmarshalling.
  • Modified SimpleProcessor to be able to control the behaviour
  • Modified VersionedAPIProcessor, and TemplateProcessor to pass strict false.
  • Added unit tests for LoadFunctionConfig to cover scenarios for strict mode, including successful loads and failure cases for unknown fields.

Copy link

linux-foundation-easycla bot commented Jun 10, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: lorenzofelletti / name: Lorenzo Felletti (e963d62, fcf54f5)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 10, 2025
@k8s-ci-robot
Copy link
Contributor

Welcome @lorenzofelletti!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 10, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @lorenzofelletti. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 10, 2025
…pleProcessor

- Introduced a new boolean field `Strict` in the SimpleProcessor struct to control strict unmarshalling behavior when loading function configurations.
- Updated the LoadFunctionConfig function to accept the `strict` parameter, allowing it to use strict unmarshalling.
- Modified SimpleProcessor to be able to control the behaviour
- Modified VersionedAPIProcessor, and TemplateProcessor to pass strict false.
- Added unit tests for LoadFunctionConfig to cover scenarios for strict mode, including successful loads and failure cases for unknown fields.
@lorenzofelletti lorenzofelletti force-pushed the processors-strict-load-config branch from 5403069 to fcf54f5 Compare June 10, 2025 09:14
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 10, 2025
@lorenzofelletti lorenzofelletti changed the title feat: add strict unmarshalling options for LoadFunctionConfig and SimpleProcessor feat: add optional strict unmarshalling for LoadFunctionConfig and SimpleProcessor Jun 10, 2025
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2025
@k8s-ci-robot
Copy link
Contributor

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 1, 2025
Copy link
Member

@stormqueen1990 stormqueen1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there, @lorenzofelletti!

Thanks for your contribution!

Could you please explain a little bit further the motivation for this change, such as a specific error mode or scenario?

@stormqueen1990
Copy link
Member

/test all

@koba1t koba1t self-requested a review August 24, 2025 16:44
@koba1t koba1t closed this Aug 24, 2025
@koba1t koba1t reopened this Aug 24, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lorenzofelletti
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@lorenzofelletti
Copy link
Author

Hi @stormqueen1990!

I'm developing a KRM function in which I'd like the configuration not to allow extra fields, e.g. I want to avoid users inadvertently doing X instead of Y:

# X
spec:
  fieldA:
    fieldB: true
    fieldC: 4
# Y
spec:
  fieldA:
    fieldC: 4
  fieldB: true

If that happens, fieldB would be defaulted in Golang to false, and no indication that the nesting is actually incorrect will be given to the user. This might lead to a function generating a configuration which is not what the user intended, and the user has to dig to understand the reason of this behaviour, until they'll find the indentation error is causing this.

It'd be much faster if an incorrect configuration just fails because of serialisation error.

To achieve this behaviour, one currently has to filter the ResourceList items, find the config manifest, serialise it, deserialise it again with the strict option, fail if the deserialisation errors.

By adding this option as a function configuration parameter, it is much easier for the function's developers to achieve this behaviour out-of-the-box.

I've set the feature to default to false for backwards compatibility, but I actually think this behaviour should be the default one, but I might be wrong in that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants